-
Notifications
You must be signed in to change notification settings - Fork 1.2k
add musl and glibc wrappers for getdents{,64} #4522
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
d6e29f6
to
5d4300a
Compare
I do not understand why freebsd tests would be failing if I have not modified any code or semver tests for freebsd. It appears the tests are failing on methods I have not modified (https://cirrus-ci.com/task/5573619788546048):
|
a5a28a1
to
f16f1da
Compare
Ok, now I'm getting a failure in a musl shard (
The Any pointers on how one would typically debug this would be appreciated. I know the header |
Ok, after testing this locally against a separate project (since the |
Ok, it looks like the freebsd shards are failing on |
Sorry this took a while to get to. Could you explain why this is needed? The manpage says:
Which sounds deprecated |
For the followup needed, |
Reminder, once the PR becomes ready for a review, use |
Some changes occurred in OpenBSD module cc @semarie |
3ffae61
to
0ea4693
Compare
0ea4693
to
f91b045
Compare
I'm very sorry for the noise. I had rebased off an incorrect branch somehow. @rustbot ready @tgross35: The man page you looked up describes the syscall interface, not the libc wrapper method, which is instead named Update: BSD supportIn the meantime I have been using my own wrappers for this and found that the BSDs also support a As before, I have provided specific links to the header files or man pages as the PR form requests. The reason POSIX standardized it is because everyone else had done so for many years. CI IssuesWhen I run tests locally, I am actually getting an error for tests I don't believe I changed (the new local test failure for
CI failure excerpt:
|
f91b045
to
87ca6bc
Compare
the musl bundled with rust appears to be from january 2024
acb184c
to
ee6b7ee
Compare
I specifically described an inability to run local testing with From reviewing recently merged PRs (https://github.com/rust-lang/libc/pulls?q=is%3Apr+is%3Aclosed), I can see that several changes were made to I see that the most recent commit to main also fails on the same freebsd-13 shard: d2ece10. So it appears that the tests are not helpful at all, and my change is passing all of the CI that everything else is. @tgross35 @semarie I have provided detailed links to manual pages. @tgross35, your first reply did not read the links I provided (according to the PR submission instructions) (along with a lurid degree of context), and instead you misinterpreted a link you searched on the web. There has since been no other responses. I see other changes such as #4729 with absolutely no information getting waved on through.
Please help me understand how to progress this in order to improve the performance of the rust stdlib. If there is something missing here, I am very willing to do more work, but right now I don't know how to proceed and the only feedback I have received did not even read the links I provided. A 2024 POSIX standard is the opposite of deprecated and this method has been available in the same stable ABI across all these platforms for at least a decade. I am also fine waiting, if it's necessary to block on CI fixes first. But this is blocking my further work on rustc and it's an exceptionally stable ABI across a wide variety of rust's supported platforms. @rustbot ready |
The Regarding how to make progress on the PR, I couldn't really help you. A friendly ping (like you did) is the way. Regarding #4729 you commented, the PR is part of a previous discussion I had with the OP (and I know him), I formally approved the PR as it touch only OpenBSD (and I agree that the PR description was very low and it is why I added some context in my comment). But I am not in position to merge it, so it will wait for someone else to merge it, as your current PR. Regards. |
Thanks so much for explaining that context. It is very reassuring to have this explanation and I appreciated your patience in describing how to understand these interactions. ^_^ |
Description
The method
posix_getdents()
has recently been added in POSIX issue 8 (from 2024). This method offers a standard interface to retrieve multiple directory entries at once into a preallocated buffer. This typically translates directly to a syscall using the existingSYS_getdents{,64}
key.However, as
posix_getdents()
is so new, it has not been universally implemented yet. musl added it immediately last year, but glibc does not yet have it. Instead, glibc >= 2.30 has the methodgetdents64()
, which is Linux-only.While musl appears to implement
posix_getdents()
for all supported platforms, it appears that the version of musl rust pulls in is not from the local machine, but from its own bundled copy. This copy does not haveposix_getdents()
, but instead simply hasgetdents64()
to match glibc on linux, as well as agetdents()
macro for compatibility with the BSD standard.The extant BSDs just name their version of this method
getdents()
, without any64
extension, and expose this as a libc method call instead of a syscall.Surprisingly, macOS does not support any form of the
getdents*()
method. I mentioned this in passing to an Apple engineer a few weeks ago.I have been in contact with a glibc developer who plans to support this, but cannot make any timeline guarantees. Since it appears that the BSDs also have yet to add support for the POSIX standardized
posix_getdents()
too, it does not seem terribly pressing to block this PR until upstream Rust pulls in a more recent version of musl. Instead, a followup PR should be able to triumphantly addposix_getdents()
to all the POSIX platforms at once! ^_^ :DSources
posix_getdents()
): https://github.com/bminor/musl/blob/86373b4999bfd9a9379bc4a3ca877b1c80a2a340/src/dirent/posix_getdents.c#L6getdents()
): https://github.com/bminor/musl/blob/86373b4999bfd9a9379bc4a3ca877b1c80a2a340/src/linux/getdents.c#L6getdents64()
): https://github.com/bminor/musl/blob/86373b4999bfd9a9379bc4a3ca877b1c80a2a340/include/dirent.h#L80getdents64()
): https://github.com/bminor/glibc/blob/7eed691cc2b6c5dbb6066ee1251606a744c7f05c/sysdeps/unix/sysv/linux/bits/dirent_ext.h#L29getdents()
): https://man.freebsd.org/cgi/man.cgi?query=getdents&sektion=2&manpath=freebsd-release-portsgetdents()
): https://man.netbsd.org/getdents.2getdents()
): https://man.openbsd.org/getdents.2getdents()
): https://man.dragonflybsd.org/?command=getdents§ion=ANYChecklist
libc-test/semver
have been updated*LAST
or*MAX
areincluded (see #3131)
cd libc-test && cargo test --target mytarget
);especially relevant for platforms that may not be checked in CI
@rustbot label +stable-nominated